-
Notifications
You must be signed in to change notification settings - Fork 78
Add SplitExplicitStepper with decoupled spin-down test #1257
Conversation
bors try |
tryBuild failed: |
bors try |
tryBuild failed: |
src/Numerics/DGMethods/DGModel.jl
Outdated
nrealelem = length(topology.realelems) | ||
nhorzrealelem = div(nrealelem, nvertelem) | ||
|
||
return (Nq, Nqk, Nfp, Np, nvertelem, nhorzelem, nhorzrealelem, nrealelem) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would a named tuple be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably
|
||
LSRK2N = LowStorageRungeKutta2N | ||
|
||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a custom / home baked stepper might be worth documenting mathematically what it's doing. (Just thinking that our future selves will need to understand it, and we will forget.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I correct that this is a very simple first order splitting of the ODE?
dQ = f(Q,P)
dP = g(Q, P)
and you are doing something like:
dQ = f(Q, P^n)
to take Q^n
to `Q^{n+1} and then
dP = g(Q^{n+1}, P)
to take P^n
to P^{N+1}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the latest commit 287c821 shows the details of the splitting by filling in the communication functions. there's still some work needed before this can be used, because we need to add two auxiliary balance laws in order to do the vertical integrals needed, but the layout of what is happening is there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will address in #1268
end | ||
|
||
# Compute (and RK update) slow tendency | ||
slow.rhs!(dQslow, Qslow, param, slow_stage_time, increment = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can replace this with something like
slow.rhs!(dQslow, Qslow, param, slow_stage_time, increment = true) | |
dQslow .+= dQ2fast |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(unless Qslow
is modified in tendency_from_slow_to_fast
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jm-c can you test this change in your branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any glaring problems.
Not sure there are enough comments to help us remember what the goal of everything is.
(Trying to think of our future selves here)
I think most of your questions/concerns about comments are about the parts of the timestepper that are stubs in this PR and will be filled out in the next one. I can also just wait and include those in this PR. I mostly wanted this decoupled test in first, but I guess it will be in all the CI even if I add the more complicated stuff here. |
bors try |
b4a6daf
to
8e8c1c8
Compare
bors r+ |
Merge conflict. |
8e8c1c8
to
3de0ae3
Compare
bors r+ |
3de0ae3
to
f4ec39c
Compare
Canceled. |
bors r+ |
f4ec39c
to
5da8511
Compare
Canceled. |
port over MultirateMultistateRK solver as SplitExplicitSolver decoupled models run, slow model matches analytical solution Revert additional steps code, fast model converges properly, add tests port bugfixes from CliMA#1256 Incorporate Jeremy's comments fix docstring Co-Authored-By: Jean-Michel Campin <jm-c@users.noreply.github.com> Co-Authored-By: sandreza <andrenogueirasouza@gmail.com> Co-Authored-By: Jeremy E Kozdon <jekozdon@nps.edu> finish rebase
5da8511
to
1b833e5
Compare
bors r+ |
1257: Add SplitExplicitStepper with decoupled spin-down test r=blallen a=blallen # Description This PR adds the MultistateMultirateRungeKutta solver from the branches `bla/multistate_multirate` and `jmc/split_explicit_dvlp` in a new form as the SplitExplicitStepper. A simple test of running the `HBModel` and `SWModel` in parallel performing the hydrostatic spindown test. Both models converge to the analytic solution. A separate PR will be added that couples the two models with this test. Originally, I attempted to include a working version of the `add_fast_steps` option from `jmc/split_explicit_dvlp`; however, that caused the `SWModel` to not converge properly. This feature will be revisited in a future PR. Co-authored-by: Brandon Allen <ballen@mit.edu>
bors r- |
Canceled. |
@charleskawczynski damn, I was trying to avoid shenanigans from #1266 and instead got shenanigans from #1050. Well at least this PR is finished and the active development is elsewhere now :) |
Sorry, we should be able to merge right after #1273. |
bors r+ |
1268: Couple the HBModel and SWModel for use in the Split-Explicit Stepper r=blallen a=blallen # Description A continuation of #1257, adding the necessary communication between the two models needed to have a true split-explicit stepper. The work in this PR enables us to run with up to a 90-minute timestep for the hydrostatic spindown test case. The broadcasts of reshaped MPIStateArrays depends on #1071 to run on the GPU. I'll pull over the commits from that PR for running bors, but we need to merge that before we merge this one. There's quite a bit of partial work in this PR that was necessary to get things running, but that either isn't fully implemented everywhere it should be or isn't implemented super elegantly (mainly regarding collecting all the Ocean models into a single module and how multiple dispatch is used for the couple). I'll revisit that once we have the full timestepping machinery running. Co-authored-by: Brandon Allen <ballen@mit.edu>
1268: Couple the HBModel and SWModel for use in the Split-Explicit Stepper r=blallen a=blallen # Description A continuation of #1257, adding the necessary communication between the two models needed to have a true split-explicit stepper. The work in this PR enables us to run with up to a 90-minute timestep for the hydrostatic spindown test case. The broadcasts of reshaped MPIStateArrays depends on #1071 to run on the GPU. I'll pull over the commits from that PR for running bors, but we need to merge that before we merge this one. There's quite a bit of partial work in this PR that was necessary to get things running, but that either isn't fully implemented everywhere it should be or isn't implemented super elegantly (mainly regarding collecting all the Ocean models into a single module and how multiple dispatch is used for the couple). I'll revisit that once we have the full timestepping machinery running. Co-authored-by: Brandon Allen <ballen@mit.edu>
Description
This PR adds the MultistateMultirateRungeKutta solver from the branches
bla/multistate_multirate
andjmc/split_explicit_dvlp
in a new form as the SplitExplicitStepper. A simple test of running theHBModel
andSWModel
in parallel performing the hydrostatic spindown test. Both models converge to the analytic solution. A separate PR will be added that couples the two models with this test.Originally, I attempted to include a working version of the
add_fast_steps
option fromjmc/split_explicit_dvlp
; however, that caused theSWModel
to not converge properly. This feature will be revisited in a future PR.I have
tests/runtests.jl
julia .dev/climaformat.jl .
For review by CLIMA developers
julia .dev/format.jl
has been run in a separate commit.